dynamic: Refactor logic to perform multiple updates #1702
Conversation
|
Please rebase this series. I think it should detect non-standard prototypes like |
clementguidi
force-pushed
the
dynamic-runtime-init
branch
from
faefcfa
to
a1e4e13
Compare
clementguidi
force-pushed
the
dynamic-runtime-init
branch
from
a1e4e13
to
2e2f31e
Compare
|
The previous three comments are now addressed. |
| else if (mdinfo && !mdinfo->next) /* main binary loaded, no module loaded */ | ||
| fmd.main_loaded = true; | ||
| else | ||
| return; |
There was a problem hiding this comment.
This condition check is confusing as it sometimes checks code_hmap and sometimes mdinfo. I think you can just call dl_iterate_phdr() always and check the mdinfo list inside to prevent duplication.
There was a problem hiding this comment.
There are performance implications if we traverse the mdinfo linked list every time we want check for a shared library. Therefore, I decided to add a new static variable 'modules_loaded' which takes this into account and will not run callback if modules are already loaded.
azharivs
force-pushed
the
dynamic-runtime-init
branch
2 times, most recently
from
1752826
to
3b6e832
Compare
| if (needs_modules) | ||
| hash_size *= 2; | ||
| code_hmap = hashmap_create(hash_size, hashmap_ptr_hash, hashmap_ptr_equals); | ||
| } |
There was a problem hiding this comment.
What happens if the first update is for main only and the next one needs modules. I think hash size would be kept only for main, right?
There was a problem hiding this comment.
Yes I spotted that and looked strange to me as well. I'll fix it.
libmcount/dynamic.c
Outdated
|
|
||
| dl_iterate_phdr(find_dynamic_module, &fmd); | ||
| modules_loaded = !dl_iterate_phdr(find_dynamic_module, &fmd); |
There was a problem hiding this comment.
I'm not sure if this is correct. IIRC the return value is from the (last) callback. Maybe you can set it only if it needs modules. How about this?
if (code_hmap) {
/* main executable should be loaded already */
if (!needs_modules || modules_loaded)
return;
/* maybe increase the hash map size */
}
else {
if (needs_modules)
hash_size *= 2;
code_hmap = hashmap_create(...);
}
dl_iterate_phdr(...);
main_loaded = true;
if (needs_modules)
modules_loaded = true;
azharivs
force-pushed
the
dynamic-runtime-init
branch
3 times, most recently
from
7830bfa
to
68993aa
Compare
| else if (!fmd->needs_modules) | ||
| return main_loaded; | ||
| else if (modules_loaded) | ||
| return 0; |
There was a problem hiding this comment.
Shouldn't it return 1? Anyway, I think we don't call this function if modules_loaded set already.
There was a problem hiding this comment.
This statement will be executed if
the current shared object is NOT the executable AND we need modules AND modules are loaded
I had this because of the original way I had the logic working, at this point it is irrelevant espacially if we don't check the return status of dl_iterate. However, I would not like to speculate and play it safe so I decided not to return an error code here.
There was a problem hiding this comment.
The prepare_dynamic_update() now checks modules_loaded and don't call dl_iterated_phdr() if it's set. So I guess the return value doesn't matter.
libmcount/dynamic.c
Outdated
|
|
||
| code_hmap = hashmap_create(hash_size, hashmap_ptr_hash, hashmap_ptr_equals); | ||
| int rc = dl_iterate_phdr(find_dynamic_module, &fmd); |
There was a problem hiding this comment.
I think it always return 0 when needs_modules is true. On error cases, it'd exit.. then it doesn't need to check the return value, right?
There was a problem hiding this comment.
correct, but checking return code to be on the safe side. I am not sure about how dl_iterate_phdr is implemented. We will be able to catch a case where dl_iterate returns non-zero for some other reason not covered by the call back. Maybe that never happens, and may be it will?!
There was a problem hiding this comment.
The man page says it'd return the value from the last callback.
|
Also, can you please rebase onto the current master when you update? |
azharivs
force-pushed
the
dynamic-runtime-init
branch
from
68993aa
to
850d43c
Compare
|
rebased to namhyung:master |
There was a problem hiding this comment.
Have you tested this PR? I'm seeing new test failures
$ make -j8 runtest TESTARG=dynamic
TEST test_run
Start 15 tests with 8 worker
Compiler gcc clang
Runtime test case pg finstrument-fu fpatchable-fun pg finstrument-fu fpatchable-fun
------------------------: O0 O1 O2 O3 Os O0 O1 O2 O3 Os O0 O1 O2 O3 Os O0 O1 O2 O3 Os O0 O1 O2 O3 Os O0 O1 O2 O3 Os
136 dynamic : OK OK OK OK OK SK SK SK SK SK OK OK OK OK OK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK
138 kernel_dynamic : SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK
139 kernel_dynamic2 : SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK
140 dynamic_xray : OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK
223 dynamic_full : NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ
224 dynamic_lib : NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ
225 dynamic_size : NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ
232 dynamic_unpatch : OK OK OK OK OK SK SK SK SK SK OK OK OK OK OK SK SK SK SK SK SK SK SK SK SK SK SK SK SK SK
233 dynamic_unpatch2 : NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ NZ
248 dynamic_dlopen : NG NG NG NG NG NG NG NG NG NG NG NG NG NG NG NG NG NG NG NG NG NG NG NG NG NG NG NG NG NG
263 patchable_dynamic : OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK
264 patchable_dynamic2 : OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK
265 patchable_dynamic3 : OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK
266 patchable_dynamic4 : OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK
267 patchable_dynamic5 : OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK OK
runtime test stats
====================
total 450 Tests executed (success: 44.44%)
OK: 200 Test succeeded
OK: 0 Test succeeded (with some fixup)
NG: 30 Different test result
NZ: 120 Non-zero return value
SG: 0 Abnormal exit by signal
TM: 0 Test ran too long
BI: 0 Build failed
LA: 0 Unsupported Language
SK: 100 Skipped
| else if (!fmd->needs_modules) | ||
| return main_loaded; | ||
| else if (modules_loaded) | ||
| return 0; |
There was a problem hiding this comment.
The prepare_dynamic_update() now checks modules_loaded and don't call dl_iterated_phdr() if it's set. So I guess the return value doesn't matter.
libmcount/dynamic.c
Outdated
|
|
||
| code_hmap = hashmap_create(hash_size, hashmap_ptr_hash, hashmap_ptr_equals); | ||
| int rc = dl_iterate_phdr(find_dynamic_module, &fmd); |
There was a problem hiding this comment.
The man page says it'd return the value from the last callback.
|
Current master has fewer failures: |
|
Actually v0.14 has no failures: |
|
The following errors are fixed by #1799, but not merged yet. |
This is in preparation of runtime dynamic patching. This commit guarantees that dynamic info is read only once for the target binary and for each module. Co-authored-by: Gabriel-Andrew Pollo-Guilbert <[email protected]> Signed-off-by: Clément Guidi <[email protected]> Signed-off-by: Seyed-Vahid Azhari <[email protected]>
If 'mcount_dynamic_update' is called multiple times (e.g. at runtime), it initializes the size filter only once. Signed-off-by: Clément Guidi <[email protected]>
Skip the initialization of the disassembly engine with it has already been performed. Signed-off-by: Clément Guidi <[email protected]>
After patching, clear the pattern list for reuse. However, archive its content in a separate aggregated list. This list contains all applied patterns and is used applied again when when opening a dynamic library. Co-authored-by: Gabriel-Andrew Pollo-Guilbert <[email protected]> Signed-off-by: Clément Guidi <[email protected]>
The 'mcount_dynamic_update' is now safe to call multiple times, including at runtime. On each call, it will perform patching and unpatching of the target (not implemented yet). Signed-off-by: Clément Guidi <[email protected]>
Install a trampoline for each loaded module map, on initialization. Keep the trampolines in memory to allow for dynamic patching at runtime. Clear the trampolines on libmcount exit. Co-authored-by: Gabriel-Andrew Pollo-Guilbert <[email protected]> Signed-off-by: Clément Guidi <[email protected]>
Use a global flag to indicate the state of the target. When the target is not running, tasks such as dynamic patching can be performed with less constraints. If libmcount.so is dynamically injected (not implemented yet), the 'mcount_target_running' flag indicates that libmcount has to be initialized in a running target. Signed-off-by: Clément Guidi <[email protected]>
Trigger architecture specific dynamic initialization when initializing the dynamic instrumentation mechanics. Co-authored-by: Gabriel-Andrew Pollo-Guilbert <[email protected]> Signed-off-by: Clément Guidi <[email protected]>
Don't save instructions if they are already present in the code hmap. Signed-off-by: Clément Guidi <[email protected]>
clementguidi
force-pushed
the
dynamic-runtime-init
branch
from
850d43c
to
c4ba8e1
Compare
|
Thanks for the update, but now I'm seeing some segfaults in the |
|
I'm investigating the segfaults. I added a commit to maintain an aggregated list of patch patterns, to apply it to DLLs, which is causing the problem I think. |
|
The |
|
Sorry for the late reply. |
|
Running with gdb shows this. It seems the value of rax register was changed. |
|
I'm still not able to reproduce the failure. Do you mind sharing the environment you're using? Maybe I can run a VM with the same config? |
|
It's a custom debian (testing) with some company internal changes. But I think it depends on toolchains. |
|
Ok, I think I found the problem. It seems dynamic tracing on In my environment, it caused the problem only if I trace libabc_test_lib and libfoo together. And I found that dlopen loaded the two libraries at an exactly same address. So when the second library patches the function, it finds the entry in the hashmap and won't add a new entry. Then later when it executes the function, it finds an entry for the previous function (which is unloaded already) and run the wrong code... boom! |
This is the first PR in a series of patches intended to bring runtime dynamic tracing on x86_64 to uftrace.
First, we refactor the life cycle of dynamic structures so a dynamic update can be triggered at anytime during execution, any amount of times. This will allow the agent to call
mcount_dynamic_update()multiple times at runtime.Related: #1698